[No QA] ci: Add Sentry build size analysis#82952
[No QA] ci: Add Sentry build size analysis#82952rinej wants to merge 22 commits intoExpensify:mainfrom
Conversation
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@jayeshmangwani no C+ review needed here |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
roryabraham
left a comment
There was a problem hiding this comment.
Ok, looks like this worked for iOS but not Android:
https://expensify.sentry.io/explore/releases/?project=4510228107427840&statsPeriod=24h&tab=mobile-builds
Also, can we add a GitHub workflow annotation (with a ::notice:: log) to link to the uploaded Sentry builds?
Also, NAB --log-level debug is more verbose than I imagined
|
|
|
Great that we have it for ios. Here is also some useful bundle chart, also the potential savings recommendations looks good -> https://expensify.sentry.io/preprod/size/52159/?project=4510228107427840 For the Android issue, the main problem is that it was uploaded with the error: From the CI logs, we can see that the APK was built and uploaded successfully, and the file size is valid (~174MB). Most likely, the failure occurs when Sentry tried to parse the binary AndroidManifest.xml. Sentry recommends using sentry-cli >= 2.58.2 for the build upload command. The workflow was using version 2.58.0 (resolved transitively via @sentry/webpack-plugin), which might be the cause of the issue. I added @sentry/cli as a direct devDependency (pinned to 3.2.0) in package.json |
|
I also added notice logs for better visibility. I know the logs are quite verbose, but once we confirm the flow works as expected, we can remove the |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@rinej can we get an actual table with proper hyperlinks like we see in the existing build summary?
|
|
Also, it looks like unfortunately the Android build size analysis failed again |
Codecov Report✅ All modified and coverable lines are covered by tests. |
Let's do both! |
|
Ok, I updated the PR to build AAB using Rock - with that we can test if that was the main issue with Sentry. I also adjusted the refactor with splitting the Sentry upload logic into separate buildAndroid and buildIOS files. there might be an issue with downloading adhoc builds in AAB format for adhoc, but let's first verify if this fixes the main issue with Sentry. If yes, we can add conversion |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
The upload was successful, so the issue is with Sentry parsing APK for android. When uploaded AAB it worked fine. For release we upload AAB by default so no additional changed there are needed, with that tested I think we can proceed with the release upload from that PR -> #82937 For add-hocs we need to find another solution, cause we need APK so it is easier to download and install on testing devices. |
|
For the adhocs, the only solution I have for now is to keep the Rock build with the APK as it is (we need the APK for the S3 upload). And add another step to build the AAB. It adds some extra build time, but when Rock uses the cache build, it shouldn’t take too long. We can remove this extra step once Sentry fixes the APK parsing |
|
I think short term that's ok. I posted further thoughts in the #expert-contributors slack room here |
roryabraham
left a comment
There was a problem hiding this comment.
Can you please combine #82937 into this PR and close that other one? I've lost sight of why they're separate and I think I'd have a clearer picture of the changes if they were combined.
.github/workflows/buildAndroid.yml
Outdated
| if: ${{ inputs.variant == 'Adhoc' }} | ||
| run: echo "ARTIFACT_URL=$ARTIFACT_URL" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Rock Remote Build - Android AAB (Sentry size analysis, AAB format is required) |
There was a problem hiding this comment.
Rather than building an .aab separately, can we just try always building an .aab in the rock action above? I looked at the source code and bit and it suggests we should be able to distribute AdHoc builds with .aab just fine: https://github.com/callstackincubator/android/blob/4cedf4d9b5c167452c96fe67233577e0fde9a025/action.yml#L387-L394
Then we can convert the .aab into an .apk below (we already do that). and we can upload the .aab from the main build into Sentry
There was a problem hiding this comment.
That was my initial thought, but I was pretty sure that for aab for adhocs we won't be able to easily download and install the app. But lets verify it, I added the change
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|

Explanation of Change
Uploads Android and iOS AdHoc builds to Sentry for size analysis after the Rock Remote Build step
Uses the ARTIFACT_PATH env var set by the Rock actions:
Android:
https://github.com/callstackincubator/android/blob/4cedf4d9b5c167452c96fe67233577e0fde9a025/action.yml#L274
iOS:
https://github.com/callstackincubator/ios/blob/dd30f7e53eee2ea6a59509793d0a30fbb5c91216/action.yml#L372
Fixed Issues
$ #82850
PROPOSAL:
Tests
testBuild.ymlfrom this branchOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari